fix(client): preserve scopes across authorization retries#2448
fix(client): preserve scopes across authorization retries#2448mattzcarey wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: e87cd73 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
There was a problem hiding this comment.
I didn't find bugs in the implementation, but this changes OAuth scope-request behavior across several auth paths (notably unioning in PRM scopes_supported and provider default scope during step-up, and forcing reauthorization in withOAuth), so it deserves a human look at the policy implications.
Extended reasoning...
Overview
This PR modifies the OAuth retry/step-up handling in the client package: handleOAuthUnauthorized now unions the stored token scope with accumulated transport scope and the challenge scope, _stepUpAuthorize in the Streamable HTTP transport additionally unions protected-resource-metadata scopes_supported and the provider's default clientMetadata.scope, both transports preserve accumulated scope/resource-metadata URL instead of overwriting them on 401, and withOAuth now computes the scope union and forces reauthorization when the union strictly exceeds the granted scope. Tests are added for each path (Streamable HTTP, legacy SSE, withOAuth, handleOAuthUnauthorized), plus a changeset and a migration-doc update that matches the new behavior.
Security risks
This is authorization-flow code, so it is inherently security-sensitive. The main policy question is scope widening: including PRM scopes_supported and the provider's default clientMetadata.scope in the step-up union means the client may request broader permissions than the specific challenge requires, which trades least-privilege for not losing previously granted permissions. That is a deliberate design choice stated in the PR description and documented in the migration guide, but it is the kind of behavioral/security decision a maintainer should confirm rather than a bot. The forceReauthorization addition in withOAuth also changes when the refresh-token path is bypassed. I did not find injection, token-leak, or bypass issues in the diff itself.
Level of scrutiny
High. The change touches auth code paths in a published SDK (401 handling, 403 step-up, legacy SSE, and the fetch middleware), and the behavior change (broader scope requests, refresh bypass) affects every OAuth-enabled client. This is well beyond the config-tweak/mechanical threshold for shadow approval.
Other factors
The implementation is internally consistent: resourceMetadataUrl ?? this._resourceMetadataUrl and computeScopeUnion(this._scope, scope) are applied uniformly across the 401 sites, the new UnauthorizedContext fields are optional and backward compatible, and the docs/changeset text matches the code. Test coverage for the new behavior is reasonable. The bug-hunting system found no issues, and there are no prior reviewer comments to weigh.
8499a5f to
9d3352f
Compare
felixweinberger
left a comment
There was a problem hiding this comment.
Thank you! Good find.
- force fresh authorization when a 401 widens the granted scope - cover refresh-token scope widening with a regression test
| const challenge = extractWWWAuthenticateParams(ctx.response); | ||
| const tokens = await provider.tokens(); | ||
| const unionScope = computeScopeUnion(tokens?.scope, ctx.scope, challenge.scope); | ||
| const forceReauthorization = isStrictScopeSuperset(unionScope, tokens?.scope); | ||
| const result = await auth(provider, { | ||
| serverUrl: ctx.serverUrl, | ||
| resourceMetadataUrl, | ||
| scope, | ||
| resourceMetadataUrl: challenge.resourceMetadataUrl ?? ctx.resourceMetadataUrl, | ||
| scope: unionScope, | ||
| ...(forceReauthorization ? { forceReauthorization } : {}), |
There was a problem hiding this comment.
🔴 handleOAuthUnauthorized computes forceReauthorization = isStrictScopeSuperset(unionScope, tokens?.scope) with no guard for the case where the stored token has no scope field — which RFC 6749 §5.1 permits (and many ASes do) when granted == requested. Since the transports now thread accumulated _scope/challenge scope into ctx.scope on every 401, an ordinary access-token expiry then bypasses the working refresh token and forces interactive re-authorization (or throws UnauthorizedError), a regression from the pre-PR silent-refresh behavior. Mirror the guard this same PR adds in withOAuth (lastTokenScope !== undefined && isStrictScopeSuperset(...)), or reconstruct the prior requested scope the way _stepUpAuthorize does before comparing.
Extended reasoning...
What the bug is
handleOAuthUnauthorized (auth.ts:186-194) now computes:
const unionScope = computeScopeUnion(tokens?.scope, ctx.scope, challenge.scope);
const forceReauthorization = isStrictScopeSuperset(unionScope, tokens?.scope);isStrictScopeSuperset (auth.ts:638-645) treats an undefined/absent current scope as the empty set, so it returns true whenever unionScope is non-empty and the stored token record has no scope field. Per RFC 6749 §5.1, authorization servers commonly omit scope from the token response when the granted scope equals the requested scope, and exchangeAuthorization/saveTokens store exactly what the AS returned — so tokens?.scope === undefined is the normal steady state for many providers, not an edge case.
The code path that triggers it
This PR newly threads the transports' accumulated _scope (and resourceMetadataUrl) into ctx.scope on every 401 for Streamable HTTP _send/_startOrAuthSse and legacy SSE, all of which route through adaptOAuthProvider → handleOAuthUnauthorized. So the trigger only requires that any earlier WWW-Authenticate challenge carried a scope param (e.g. the very first pre-token 401 that kicked off authorization — RFC 6750 servers routinely include one), or that the current 401 does.
Step-by-step regression
- Initial connection: server returns
401withWWW-Authenticate: Bearer scope=\"read\"→ transport records_scope = 'read', user completes interactive auth. - Token response omits
scope(granted == requested, per §5.1) → stored tokens are{ access_token, refresh_token }with noscopefield. - Later, the access token expires; the server returns a routine
401. - Transport calls
onUnauthorizedwithctx.scope = 'read'(new in this PR) →unionScope = 'read',tokens?.scope = undefined→isStrictScopeSuperset('read', undefined)→true→forceReauthorization: true. authInternal(auth.ts:1313) skips therefresh_tokenbranch entirely whenforceReauthorizationis set and starts a fresh authorization request; for interactive providers that callsredirectToAuthorizationand returns'REDIRECT', sohandleOAuthUnauthorizedthrowsUnauthorizedErrorand the user is bounced through the consent flow on every ordinary token expiry.
Before this commit, handleOAuthUnauthorized never passed forceReauthorization, so step 3–5 was handled by a silent refresh + retry. This is therefore a concrete behavioral regression introduced here, not a pre-existing quirk.
Why the existing code doesn't prevent it
The isStrictScopeSuperset doc comment describes its "conservative" treatment of an absent token scope, but that reasoning was written for the 403 insufficient_scope step-up path, where the server has explicitly said the current grant is insufficient and forcing a fresh authorization is the right call. A plain 401 is overwhelmingly token expiry, where the refresh token is exactly the right tool. The same PR demonstrates awareness of this distinction in the sibling paths: withOAuth (middleware.ts:66) guards with lastTokenScope !== undefined && isStrictScopeSuperset(...), and _stepUpAuthorize reconstructs the previously-requested scope from discoveryState/clientMetadata before comparing precisely so an omitted token scope isn't treated as "no prior grant". handleOAuthUnauthorized does neither, so the two 401 paths this PR touches are inconsistent.
Impact
For any provider whose token responses omit scope and any deployment where a challenge scope has been observed (or the transport accumulated one), every access-token expiry becomes an interactive consent bounce (browser redirect) or a hard UnauthorizedError for non-interactive clients — instead of the silent refresh that worked before this PR. This affects Streamable HTTP, legacy SSE, and the raw handleOAuthUnauthorized helper.
How to fix
Gate the force the same way withOAuth does in this PR:
const forceReauthorization = tokens?.scope !== undefined && isStrictScopeSuperset(unionScope, tokens.scope);or reconstruct the previously-requested scope (transport ctx.scope, then PRM scopes_supported, then clientMetadata.scope) the way _stepUpAuthorize does and compare the union against that, so a genuinely widened challenge still forces re-authorization while a routine expiry keeps using the refresh token.
Previously unaddressed
#2356 accumulated token, transport, and challenge scopes during 403 step-up, but could still lose earlier permissions when a token response omitted
scope. It also did not carry accumulated scope and resource metadata through automatic 401, legacy SSE, andwithOAuthpaths.This PR
withOAuthVerification
pnpm check:allpnpm build:allFollow-up to #2265, #2356, and #2286.